-
Notifications
You must be signed in to change notification settings - Fork 45
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add NavSat messages #206
Add NavSat messages #206
Conversation
Codecov Report
@@ Coverage Diff @@
## ign-msgs8 #206 +/- ##
==========================================
Coverage 84.56% 84.56%
==========================================
Files 7 7
Lines 855 855
==========================================
Hits 723 723
Misses 132 132 Continue to review full report at Codecov.
|
Signed-off-by: Louise Poubel <[email protected]>
24e380c
to
987eb5f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me with just one minor comment!
syntax = "proto3"; | ||
package ignition.msgs; | ||
option java_package = "com.ignition.msgs"; | ||
option java_outer_classname = "Protos"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be NavSatSensorProtos
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good eye! This was actually copy-pasted, there are other messages with the same problem:
$ grep -r "\"Protos\"" proto/ignition/msgs/
proto/ignition/msgs/sensor_noise.proto:option java_outer_classname = "Protos";
proto/ignition/msgs/polylinegeom.proto:option java_outer_classname = "Protos";
proto/ignition/msgs/gps_sensor.proto:option java_outer_classname = "Protos";
I'm not sure what this affects and whether fixing this could break ABI 🤔
Signed-off-by: Louise Poubel <[email protected]>
Please don't merge this until the downstream PRs are done; I want to make sure the message has everything we need before releasing. |
A couple of downstream PRs have been approved, so I think this is good to go. |
This pull request has been mentioned on Gazebo Community. There might be relevant details there: https://community.gazebosim.org/t/new-ignition-releases-2022-01-10/1228/1 |
🎉 New feature
Part of
Required by
Related to
Summary
On gazebosim/sdformat#453 we decided to use the more generic "NavSat" name for navigation satellite systems, instead of GPS, which is one specific type. This change should be reflected on the messages too.
This PR is only adding the new
NavSat
messages, but we should also update the sensors message to use it on Garden.Here's the ROS 2 NavSatFix message as a reference.
I retargeted this PR from Dome to Fortress because the GPS sensor will need to use spherical coordinates (see gazebosim/gz-sim#981).
Test it
I'm updating the downstream PRs to use this message.
Checklist
codecheck
passed (See contributing)Note to maintainers: Remember to use Squash-Merge
🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸